Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mptcp: ADD_ADDR are now sent sooner #34

Merged

Conversation

matttbe
Copy link
Member

@matttbe matttbe commented Jan 23, 2021

A recent modification of the kernel changes the way ADD_ADDR are sent.
See: multipath-tcp/mptcp_net-next#139

In consequences, we have to adapt our tests to have these ADD_ADDR sent
earlier, ASAP in fact. We now make sure ADD_ADDR and ADD_ADDR echo are
properly sent before injecting MP_JOIN SYN and SYN+ACK or sending data.

Closes: multipath-tcp/mptcp_net-next#144
Signed-off-by: Matthieu Baerts matthieu.baerts@tessares.net

@matttbe matttbe requested a review from dcaratti January 23, 2021 10:22
@@ -16,9 +16,14 @@
+0.01 < . 1:1(0) ack 1 win 257 <mpcapable v1 flags[flag_h] key[ckey=2,skey]>
+0 accept(3, ..., ...) = 4

// ADD_ADDR is sent directly (>= 5.12), not after first data (< 5.12)
+0 > . 1:1(0) ack 1 <add_address addr[saddr] hmac=auto,dss dack4=1 ssn=1 dll=0 nocs>
// TODO: support injecting ADD_ADDR echo
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linked to multipath-tcp/mptcp_net-next#142
(same for the others)

@matttbe
Copy link
Member Author

matttbe commented Jan 23, 2021

@geliangtang : may you confirm that the new behaviour is the expected one please? :)

@geliangtang
Copy link
Member

geliangtang commented Jan 23, 2021

@matttbe Sorry Matt, I don't know what dose the dack8 mean. Does it mean ack64 in DSS?

I found these lines in my kernel log:

[ 55.888996] MPTCP: send ack for add_addr
[ 55.889036] MPTCP: DSS
[ 55.889040] MPTCP: data_fin=0 dsn64=0 use_map=0 ack64=1 use_ack=1
[ 55.889044] MPTCP: data_ack=1042103240348029546

So we use ack64 in DSS indeed.

@matttbe
Copy link
Member Author

matttbe commented Jan 24, 2021

Hi @geliangtang

I don't know what dose the dack8 mean. Does it mean ack64 in DSS?

Oh yes sorry I should have clarified that. Yes it means ack64.

[ 55.888996] MPTCP: send ack for add_addr
[ 55.889036] MPTCP: DSS
[ 55.889040] MPTCP: data_fin=0 dsn64=0 use_map=0 ack64=1 use_ack=1
[ 55.889044] MPTCP: data_ack=1042103240348029546

So we use ack64 in DSS indeed.

Thank you for having looked!

Before sending the ACK with the ADD_ADDR, we didn't receive any 64bit DSN. In theory, we should only send a ACK64 if we got a 64bit DSN before. See: multipath-tcp/mptcp_net-next@a0c1d0e

@dcaratti
Copy link
Collaborator

dcaratti commented Jan 26, 2021

Hi @geliangtang

I don't know what dose the dack8 mean. Does it mean ack64 in DSS?

Oh yes sorry I should have clarified that. Yes it means ack64.

[ 55.888996] MPTCP: send ack for add_addr
[ 55.889036] MPTCP: DSS
[ 55.889040] MPTCP: data_fin=0 dsn64=0 use_map=0 ack64=1 use_ack=1
[ 55.889044] MPTCP: data_ack=1042103240348029546

So we use ack64 in DSS indeed.

Thank you for having looked!

Before sending the ACK with the ADD_ADDR, we didn't receive any 64bit DSN. In theory, we should only send a ACK64 if we got a 64bit DSN before. See: multipath-tcp/mptcp_net-next@a0c1d0e

not sure if this problem just happens after reception of ADD_ADDR. But I think we can merge this commit and then eventually change scripts again once the kernel behavior is improved. @geliangtang / @matttbe, do you think that issue multipath-tcp/mptcp_net-next#139 could also fix issue multipath-tcp/mptcp_net-next#112 ?

A recent modification of the kernel changes the way ADD_ADDR are sent.
See: multipath-tcp/mptcp_net-next#139

In consequences, we have to adapt our tests to have these ADD_ADDR sent
earlier, ASAP in fact. We now make sure ADD_ADDR and ADD_ADDR echo are
properly sent before injecting MP_JOIN SYN and SYN+ACK or sending data.

Closes: multipath-tcp/mptcp_net-next#144
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
@matttbe
Copy link
Member Author

matttbe commented Jan 26, 2021

not sure if this problem just happens after reception of ADD_ADDR. But I think we can merge this commit and then eventually change scripts again once the kernel behavior is improved.

@dcaratti : Sounds good to me!

I just found the answer to my question about the dack8, see above :)
So if it is OK for you, we can merge!

@geliangtang / @matttbe, do you think that issue multipath-tcp/mptcp_net-next#139 could also fix issue multipath-tcp/mptcp_net-next#112 ?

When re-reading the description of your issue, it could be linked but I don't think I saw this behaviour on my CI. Maybe something else needs to be adapted on your kernel?
But it maybe still makes sense for you to backport multipath-tcp/mptcp_net-next@0da285e

@dcaratti
Copy link
Collaborator

When re-reading the description of your issue, it could be linked but I don't think I saw this behaviour on my CI. Maybe something else needs to be adapted on your kernel?

I will try that reverting locally multipath-tcp/mptcp_net-next@4a41f453be, and see if I see the selftest 13 failing

@dcaratti dcaratti merged commit 7ec54eb into multipath-tcp:mptcp-net-next Jan 27, 2021
@matttbe matttbe deleted the issue-144-add_addr-sooner branch January 27, 2021 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Packetdrill: ADD_ADDR are now sent sooner
3 participants